RAII chapter for idiomatic rust #2820
Conversation
GlenDC
requested review from
LukeMathWalker,
djmitche,
gribozavr and
randomPoison
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
src/idiomatic/leveraging-the-type-system/raii/drop_limitations.md
Outdated
Show resolved
Hide resolved
src/idiomatic/leveraging-the-type-system/raii/drop_limitations.md
Outdated
Show resolved
Hide resolved
randomPoison
left a comment
randomPoison
left a comment
There was a problem hiding this comment.
Looks pretty good, there's lots of good stuff in this chapter 😁 I've got various feedback below, but it's definitely off to a good start!
|
I started to action all feedback since today. I'll explicitly re-request reviews once I am done. Probably best not to review again until then. |
- directly where possible - otherwise as inline feedback as notes to take into account for next draft
refresher on `RAII` and use the OS File Descriptor example to start the discussions around RAII all previous content is for now moved to `_old` for my own reference, will add the new content based on the agreed upon new structure next.
|
The four slides in the RAII chapter are ready for review again. |
randomPoison
left a comment
randomPoison
left a comment
There was a problem hiding this comment.
This is looking pretty good. The drop bomb and scope guard slides look good to me, my biggest suggestion is that we might want to split the mutex slide up so that it flows a bit better.
djmitche
left a comment
djmitche
left a comment
There was a problem hiding this comment.
I had a brief look at the updates since my last comments, and I'm happy enough!
split mutex (RAII) chapter in two parts
|
@randomPoison your feedback was all address as far as I know. Ready for another round of reviews. |
randomPoison
left a comment
randomPoison
left a comment
There was a problem hiding this comment.
Thanks for addressing my previous feedback! I've looked over things again and still have some suggestions/requests for how this can be improved. There's a couple of places where I've suggested adding new slides:
- Splitting out the discussion of when
dropdoes and doesn't run. - Using
mem::forgetto defuse drop bombs. - Using
Optionfor cases where you need to transfer ownership indrop.
I'm fine with those being split into a separate PR if you want to get this one landed, though if you do that then it'd be good to open an issue tracking the unfinished changes to this section.
| <https://doc.rust-lang.org/src/std/os/fd/owned.rs.html#169-196> | ||
|
|
||
| - If both `drop()` and `close()` exist, the file descriptor may be released | ||
| twice. To avoid this, remove `close()` and rely solely on `Drop`. |
There was a problem hiding this comment.
I'd like more explicit instructions for the speaker about how the code should evolve up to this point, and how to demonstrate this. In particular the order in which the instructor should add the drop method, remove the close method, how to demonstrate the double-closing of the file etc. The flow can get very messy during the actual presentation if we don't think this through ahead of time.
| - If a destructor itself panics during unwinding, the program aborts | ||
| immediately. | ||
| - If the program exits with `std::process::exit()` or is compiled with the | ||
| `abort` panic strategy, destructors are skipped. |
There was a problem hiding this comment.
This is good information, but it is not made actionable for the listener. Quoting my original comment:
It is a good tie-in to discuss use cases for drop: it is good for cleaning up things within the scope of a process, but not the right tool for guaranteeing that something happens outside of the process (e.g., on local disk, or in another service in a distributed system).
For example, it is a bad idea to rely exclusively on drop to clean up temporary files: if the program terminates in a way that skips running drop, temporary files will persist, and eventually the computer will run out of space. This can happen if the program crashes or leaks the value whose drop is responsible for deleting the file. In addition to a drop implementation within the program, one also needs a classic unix-style temp file reaper that runs as a separate process.
I suggest adding an example like that to the speaker notes. Otherwise the information "drop is not run when the program exits" is not connected to any real-world bad outcome / architectural mistake.
|
@GlenDC Thank you for the updates! Please see my latest round of comments. There are a lot of comments, but I tried to make them actionable. Please pay particular attention to the requests to split the slides. Looking forward to seeing the revised section, I think this PR is getting closer and closer to being finalized. |
5 participants
This PR adds the RAII chapter for the idiomatic Rust deep dive.